-
Notifications
You must be signed in to change notification settings - Fork 108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expose query duration via histogram #121
Conversation
Signed-off-by: Wilfried Roset <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool, thanks for the contribution! I left two small comments but otherwise it looks very nice.
config.go
Outdated
@@ -100,11 +114,16 @@ type CloudSQLConfig struct { | |||
|
|||
// File is a collection of jobs | |||
type File struct { | |||
Misc Misc `yaml:"misc,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two questions:
- What do you think about calling it Configuration or something to make it more explicit that it's a new place to put advanced configuration options.
- Would it make sense to have this part job specific and not globally available? I could imagine that there's maybe jobs that take longer where you want different buckets than just for a quick
select
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about calling it Configuration or something to make it more explicit that it's a new place to put advanced configuration options.
LGTM
Would it make sense to have this part job specific and not globally available? I could imagine that there's maybe jobs that take longer where you want different buckets than just for a quick select
If my understanding is correct the bucket definition is done at the histogram definition. If we want to have the bucket definition at the job level we need to define multiple histogram which can severely impact the cardinality. I would recommend waiting for the native histogram which are design to address such issue.
Signed-off-by: Wilfried Roset <[email protected]>
b3611f6
to
7edd749
Compare
thank you for your feedbacks which I have taken into account. |
…stalling the correct go version based on go.mod Signed-off-by: Wilfried Roset <[email protected]>
The following PR introduces histogram for query duration. Doing so allow to use sql_exporter for synthetic monitoring.
A user could the following configuration
The resulting histogram looks like this
This is of great help to monitor the latency from a client point of view and build SLO around that.
See also: https://prometheus.io/docs/practices/histograms/